See #488 (comment) about the failure

Still relevant for a few othe rclasses, @ekohl what do you think about an approach that uses:

      source => pick($server_ca, $certs::ca::server_ca_path),

And then makes $server_ca optional at the class parameter level?

And then makes $server_ca optional at the class parameter level?

Depends. Today we have code calling $certs::foreman::server_ca and if it's undef at the top level, you'll need some other way to reuse the variable.

One thing we should consider is for the server side certificates is to use bundles where both the public key and the chain is included. Then we don't pass in the CA at all anymore. We'll need to verify this when a users passes them in, but it could simplify things.

The more I think about it, I don't think that server_ca_cert parameter is necessary and it doesn't make sense with the way the certificates are intended to work and depend upon the build directory.

Partly you're right: you really need to serve the chain. The client should already have the root CA, but you do need to serve the intermediates.

For example:

Certificate chain
 0 s:CN=example.com
   i:C=US, O=Let's Encrypt, CN=R10
   a:PKEY: rsaEncryption, 4096 (bit); sigalg: RSA-SHA256
   v:NotBefore: Jan 19 10:08:30 2025 GMT; NotAfter: Apr 19 10:08:29 2025 GMT
 1 s:C=US, O=Let's Encrypt, CN=R10
   i:C=US, O=Internet Security Research Group, CN=ISRG Root X1
   a:PKEY: rsaEncryption, 2048 (bit); sigalg: RSA-SHA256
   v:NotBefore: Mar 13 00:00:00 2024 GMT; NotAfter: Mar 12 23:59:59 2027 GMT

Here you can see certificate 0 is the server certificate, which you obviously need to serve. Then we also see the intermediate CA (C=US, O=Let's Encrypt, CN=R10) that issued the server certificate, but you don't see the root CA that issues the intermediate (C=US, O=Internet Security Research Group, CN=ISRG Root X1).

What you can do is mandate the intermediate CA is included in the cert.pem file and omit the server_ca_file.

If you ever looked at what certbot does then you see it writes out 4 files:

• cert.pem contains only the server certificate
• chain.pem contains only the chain between the server certificate and the root
• fullchain.pem is both cert.pem and chain.pem concatenated
• privkey.pem is the private key

Now you can configure Apache with:

SSLCertificateFile /path/to/fullchain.pem
SSLCertificateKeyFile /path/to/privkey.pem

The deprecated way (that we today still use in our installer):

SSLCertificateFile /path/to/cert.pem
SSLCertificateChainFile /path/to/chain.pem
SSLCertificateKeyFile /path/to/privkey.pem

And SSLCACertificateFile is only for client auth.

This is pretty easy to build for our default CA: we're in control. However, we're not for what users input and I'd struggle to come up with a good way to prevent users to shoot themselves in the foot. This is why I'd love to adopt ACME, which is what Let's Encrypt came up with. These days it's also implemented by others, including dogtag (the CA component of FreeIPA).

I am writing up a "future" certificate design with the guidelines we want to abide by as part of the foreman-quadlet research repository. This is great input to that, but I don't think I should include any of it here, specifically in this PR. I am aiming to mostly cleanup this puppet module to make it easier to consume in our future states by more clearly splitting generate and deploy, and deploying certificates to the locations they are needed for the service that needs them there.

Nearly all of these service classes are not used stand-alone anywhere today, and nor were they really designed to be even though they exposed parameters that would lead one to think that. The removed parameter server_ca was really not intended to ever be used due to how generate and deploy, are tied together and how this module in general was never meant to be used with anything other than the katello-certs-tools generated certificate method. We will fix that but I want to do that somewhere cleaner than here and just make this module have cleaner generate / deploy separation for flexibility.

I get that and it was me thinking zoomed out and comparing to a desired state before zooming back in again.

Cool - thoughts then on this specific change being good to merge?